-
Notifications
You must be signed in to change notification settings - Fork 324
IndexedDB: prepare matrix-sdk-indexeddb
crate for updating indexed_db_futures
to latest version
#5467
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
IndexedDB: prepare matrix-sdk-indexeddb
crate for updating indexed_db_futures
to latest version
#5467
Conversation
…r fns Signed-off-by: Michael Goldenberg <[email protected]>
…sub-modules in crypto store Signed-off-by: Michael Goldenberg <[email protected]>
…a single upgrade in v14 Signed-off-by: Michael Goldenberg <[email protected]>
Signed-off-by: Michael Goldenberg <[email protected]>
…callback in crypto store Signed-off-by: Michael Goldenberg <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5467 +/- ##
==========================================
+ Coverage 88.88% 88.89% +0.01%
==========================================
Files 333 333
Lines 92082 92082
Branches 92082 92082
==========================================
+ Hits 81850 81860 +10
+ Misses 6403 6394 -9
+ Partials 3829 3828 -1 ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging #5467 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this well-thought-through and well-explained contribution!
It's been ages since I looked at this code, so I might be misunderstanding, but here is what I think is happening:
- we are unable to pass the
IdbTransaction
argument to the callback indo_schema_upgrade
, which means that we can't get hold of an existing object store inside aschema_add
function to add an index to it. - Therefore, we need to create a new object store and migrate across if we want to add an index to it.
Is that right?
And then you've added new version numbers that represent the creation, migration and deletion involved in creating a new object store.
If I've understood it right, then here is my response: I agree that it is inconvenient to wait for dependencies to add (reinstate!) features so I would normally be strongly in favour of working around something like this, BUT this migration will probably take a very long time for some users, and as far as I know Element Web has zero user feedback while it is going on, making for a bad user experience.
This Element Web problem is tracked here: element-hq/element-web#26892
I don't think we can add a migration like this without first seeing this issue addressed in Element Web.
Further, this change will cause a long delay for lots of users even if they are already on the latest schema version, so they already have the right index! So, it would be highly desirable to get this fixed upstream.
What do you think?
@andybalaam: thanks for your response! And yes, I believe you have understood correctly! Now my turn to make sure I am understanding you: the problem is that the web-based Element client uses the That's a rather tricky problem... I hadn't thought about how long the migration might take. So, the concern with waiting for an upstream patch to I will confer with the people at @filament-dm, as this work is being done in service of their client. Perhaps we will have to keep this in their fork until we see some movement elsewhere. Out of curiosity, do you have any concrete sense of when element-hq/element-web#26892 might be resolved? And is this something I can help with? Also, tagging @zzorba here, in case he wants to weigh in or has other ideas about how to move forward. |
@mgoldenberg yes, I think we are understanding each other very well. I'd really like to avoid a slow migration for every user when it is not needed. If Help with fixing element-hq/element-web#26892 would be great - it might need some input from Element Design team to decide how it should look, but we could probably get that, or merge a good-enough solution in the meantime. BUT the more I think about this the less I like adding a delay for all users simply because of a dependency issue. I guess part of the question is how urgent this is. Maybe we are prepared to wait a while to see whether |
Background
This pull request makes the preliminary changes necessary to update
indexed_db_futures
in thematrix-sdk-indexeddb
crate. The reason we'd like to update this dependency is because the version currently used does not fully support the Chrome browser (see #5420).The latest version of
indexed_db_futures
has significant changes. Many of these changes can be integrated without issue. There is, however, a single change which requires substantive modification to thematrix-sdk-indexeddb
crate. Namely, one cannot access the active transaction in the callback to update the database (for details, see Alorel/rust-indexed-db#66).This is only an issue in the
crypto_store
module, where migrations are provided access to the active transaction (see here). Furthermore, there is only a single migration (v11_to_v12
) in thecrypto_store
module which makes use of the active transaction (see here).For clarity, the reason
v11_to_v12
is problematic in the latest versions ofindexed_db_futures
is because it is simply adding an index to an object store which was created in a different migration and this requires access to the active transaction. All the other migrations create object stores and indices in the same migration, which does not suffer from the same issue.Changes
Due to the issues above, I have taken the following steps to prepare
matrix-sdk-indexeddb
for an update toindexed_db_futures
.v11_to_v12
migration, which could no longer be accomplished without the active transactionv13_to_v15
, which replicate the logic fromv11_to_v12
in a way that is compatible with later versions ofindexed_db_futures
.Alternatives
My guess is that this problem is an oversight in the interface developed in the
indexed_db_futures
crate and not a deeper issue - i.e., it seems it would be possible to expose the active transaction in later versions, as is the case in the current version used inmatrix-sdk-indexeddb
.Alternatives to the changes in this pull request include the following.
indexed_db_futures
to make the necessary modifications in a future versionmatrix-sdk-indexeddb
Neither of these seemed particularly expedient or attractive, so I opted for making changes to the migrations.
Future Work
indexed_db_futures
to the latest version - i.e.,0.6.4
Signed-off-by: Michael Goldenberg [email protected]